Use connection dropdown by gzdunek · Pull Request #692 · gravitational/webapps

gzdunek · 2022-03-23T12:50:23Z

Closes https://github.com/gravitational/webapps.e/issues/146

gzdunek · 2022-03-23T12:53:13Z

        px="2"
        mx="2"
-        as={NavLink}
+        as={url ? NavLink : null}


In teleterm we don't have classical routing, so rendering NavLink causes errors (that it should be used in context of Router)

ravicious · 2022-03-23T15:31:12Z

        px="2"
        mx="2"
-        as={NavLink}
+        as={url ? NavLink : null}


Could we change it so that instead of rendering a div when there's no URL it renders a button styled as a link instead? This actually provides keyboard support OOTB and helps us avoid having onClicks on divs.

Done, I had to add Flex container to the items in menu, because Button was going outside ofMenu.

ravicious · 2022-03-23T15:41:46Z

-        onClick={() => {
-          connect(serverUri);
+      <MenuSshLogin
+        menuListCss={menuListCss}


I've been here for just two months, so I suppose @kimlisa might have something more to say. But long-term, injecting arbitrary CSS seems to me like a wrong way of modifying the appearance of a shared component.

Ideally I suppose everything should work OOTB after we change the theme in Teleterm. However, I noticed that the way we use the theme is color-oriented rather than purpose-oriented.

webapps/packages/shared/components/MenuSshLogin/MenuSshLogin.tsx

Lines 130 to 137 in 0b65709

const StyledMenuItem = styled(MenuItem)(

({ theme }) => `

color: ${theme.colors.grey[400]};

font-size: 12px;

border-bottom: 1px solid ${theme.colors.subtle};

min-height: 32px;

&:hover {

color: ${theme.colors.link};

webapps/packages/design/src/Menu/MenuList.jsx

Lines 32 to 33 in 0b65709

const StyledMenuList = styled.div`

background-color: ${props => props.theme.colors.light};

I might be wrong, but I think this makes it harder to reuse components, because theme.colors.light might mean different things in different contexts. I saw a good thread about that a couple of days ago which goes into detail about this issue.

OTOH, taking care of that is probably out of scope for this PR.

One solution which comes to my mind is having a special copy of the theme in Teleterm which will override the Teleterm theme in places like this one here where we really need to bend a shared component to make it look a certain way. This means that in this context theme.colors.light could be something entirely else than it is in the general Teleterm theme. What do you think about this idea?

i honestly don't have strong opinions on this matter :)

and if i understood you correctly, i think we do something similar to what you suggested where we use our own custom colors (our console view theme is darker than our normal teleport theme):

define custom colors to use throughout console view

custom theme provider using custom colors

usage

Thanks for detailed comment, I agree!
I chose the way with css styling to apply more complicated changes like: different border radius, no separator between items, rounded corners of hovered item.
However, since we have agreed that color customization is sufficient, styling using a provider is simple.

ravicious

Ran into some problems when testing how this change impacts the web UI.

ravicious · 2022-03-24T15:14:59Z

  border-radius: 4px;
  box-sizing: border-box;
-  color: #263238;
+  color: ${theme.colors.blueGrey[900]};


I wanted to check if the change from <React.Fragment> to <Flex flexDirection="column"> doesn't change anything in the web UI, but when I try to use the web UI on this branch I get "theme.colors.blueGrey is undefined" after clicking on Connect.

Could you make sure that this change and the <React.Fragment> change don't cause problems for the web UI?

I tested that change in the web UI as well...
If only I had noticed that I am testing on localhost:3080, not localhost:8080 🙄🙄

Fixed

ravicious

Also, forgot to mention that I totally forgot that the dropdown from the Web UI looks as if it was one of our pickers (an input at the top plus a list of possible results), but works in a completely different way. 😬 The input doesn't really limit down the list of possible options and so on.

But let's see what the team thinks about it. We can change that to a filterable list or something if it turns out to be a problem.

gzdunek · 2022-03-24T16:00:32Z

Force-push to re-trigger builds

gzdunek commented Mar 23, 2022

View reviewed changes

gzdunek requested review from kimlisa and ravicious March 23, 2022 13:00

ravicious reviewed Mar 23, 2022

View reviewed changes

gzdunek force-pushed the gzdunek/use-connection-dropdown branch from 6c07d21 to 07987d0 Compare March 24, 2022 14:39

ravicious reviewed Mar 24, 2022

View reviewed changes

gzdunek force-pushed the gzdunek/use-connection-dropdown branch from 07987d0 to 0469e21 Compare March 24, 2022 15:33

ravicious approved these changes Mar 24, 2022

View reviewed changes

gzdunek force-pushed the gzdunek/use-connection-dropdown branch from 0469e21 to 8c8ce64 Compare March 24, 2022 15:47

gzdunek added 2 commits March 24, 2022 16:51

Render ssh menu item as NavLink only when URL is provided

511fee2

Use connection dropdown instead of modal for supplying SSH username

0234cda

gzdunek force-pushed the gzdunek/use-connection-dropdown branch from 8c8ce64 to 0234cda Compare March 24, 2022 15:59

gzdunek merged commit f8cfdf3 into master Mar 24, 2022

gzdunek deleted the gzdunek/use-connection-dropdown branch March 24, 2022 16:16

ravicious mentioned this pull request Apr 26, 2022

[v9] Backport Teleport Connect (2 of 5) #763

Merged

ravicious mentioned this pull request Mar 24, 2023

[Web] Refactor theme gravitational/teleport#23539

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use connection dropdown#692

Use connection dropdown#692
gzdunek merged 2 commits intomasterfrom
gzdunek/use-connection-dropdown

gzdunek commented Mar 23, 2022

Uh oh!

gzdunek Mar 23, 2022

Uh oh!

ravicious Mar 23, 2022

Uh oh!

gzdunek Mar 24, 2022

Uh oh!

ravicious Mar 23, 2022

Uh oh!

kimlisa Mar 23, 2022 •

edited

Loading

Uh oh!

gzdunek Mar 24, 2022

Uh oh!

ravicious left a comment

Uh oh!

ravicious Mar 24, 2022

Uh oh!

gzdunek Mar 24, 2022 •

edited

Loading

Uh oh!

ravicious left a comment

Uh oh!

gzdunek commented Mar 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	const StyledMenuItem = styled(MenuItem)(
	({ theme }) => `
	color: ${theme.colors.grey[400]};
	font-size: 12px;
	border-bottom: 1px solid ${theme.colors.subtle};
	min-height: 32px;
	&:hover {
	color: ${theme.colors.link};

	const StyledMenuList = styled.div`
	background-color: ${props => props.theme.colors.light};

Conversation

gzdunek commented Mar 23, 2022

Uh oh!

gzdunek Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

ravicious Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

gzdunek Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

ravicious Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

kimlisa Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzdunek Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

ravicious Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

gzdunek Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

gzdunek commented Mar 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kimlisa Mar 23, 2022 •

edited

Loading

gzdunek Mar 24, 2022 •

edited

Loading